.github/workflows/release-dev.yaml (1)

84-88: 💤 Low value

30-minute sleep incurs significant runner cost.

The debounce sleep consumes 30 minutes of GitHub-hosted runner time per push to dev. While the intent (coalescing merge storms via cancel-in-progress) is documented and valid, consider whether a shorter window (e.g., 5-10 minutes) would provide sufficient coalescing while reducing cost—especially if dev sees frequent pushes.

Alternatively, GitHub's native concurrency debouncing could be enhanced with a workflow-level delay via a scheduled dispatch pattern, though that adds complexity.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release-dev.yaml around lines 84 - 88, The "Coalesce a
burst of merges" workflow step is sleeping for 1800s (run: sleep 1800) which
wastes GitHub runner time on every push; replace that hard 30-minute sleep with
a shorter debounce (e.g., run: sleep 300 or 600 for 5–10 minutes) or remove the
sleep and rely on GitHub concurrency/cancel-in-progress semantics, and update
the surrounding comment/reset-debounce text accordingly; change the run value in
the step that has if: github.event_name == 'push' (or remove the step) so the
workflow only holds runners for a minimal debounce window.

package/Shaders/Common/DirectionalShadow.hlsli (1)

4-20: ⚡ Quick win

Make the LightLimitFix dependency explicit.

This helper calls LightLimitFix::GetDirectionalShadow but only documents the required include order in a comment. That makes future shader reuse brittle. Please include the dependency directly or wrap the LLF path in a header that owns both includes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package/Shaders/Common/DirectionalShadow.hlsli` around lines 4 - 20, The
LIGHT_LIMIT_FIX path in DirectionalShadow::GetSceneDirectionalShadow calls
LightLimitFix::GetDirectionalShadow but only relies on a comment about include
order; explicitly include the dependency by adding an `#include`
"LightLimitFix.hlsli" (or create/include a wrapper header that includes both
Common/Math.hlsli and LightLimitFix.hlsli) at the top of DirectionalShadow.hlsli
so LightLimitFix::GetDirectionalShadow is guaranteed to be available when
LIGHT_LIMIT_FIX is defined.

src/Features/LightLimitFix/ParticleLights.h (1)

3-24: ⚡ Quick win

Make this header self-contained.

ParticleLights.h uses std::string, ankerl::unordered_dense::map, and RE::NiColor, but it only includes <cstdint>. Please include the owning headers here instead of relying on transitive includes or the PCH.

Suggested includes

 `#include` <cstdint>
+#include <string>
+#include <ankerl/unordered_dense.h>

Add the repo's normal header for RE::NiColor as well.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Features/LightLimitFix/ParticleLights.h` around lines 3 - 24, The header
is not self-contained: ParticleLights declares std::string,
ankerl::unordered_dense::map and RE::NiColor but only includes <cstdint>; update
ParticleLights.h to directly include the owning headers (e.g., add <string>, the
ankerl unordered_dense header used by ankerl::unordered_dense::map, and the
project header that defines RE::NiColor) so that the types used in struct
Config, GradientConfig, and the member maps (particleLightConfigs,
particleLightGradientConfigs) are available without relying on transitive
includes or PCH.

tests/cpp/test_isl_radiusmath.cpp (1)

40-62: ⚡ Quick win

Add a regression for the exact-zero radius boundary.

The suite covers the negative-sqrt NaN path, but not the radius == 0 boundary that also needs clamping once CalculateRadius is hardened. A focused test here would lock down the crash-prone case.

Suggested test

 TEST_CASE("CalculateRadius clamps a NaN (negative-sqrt) result to 1", "[isl]")
 {
 	// intensity 0 with a large size makes the sqrt argument negative -> NaN -> 1.
 	REQUIRE(ISLMath::CalculateRadius(0.0f, false, 1.0f, 10.0f) == Approx(1.0f));
 }
+
+TEST_CASE("CalculateRadius clamps an exact-zero radius to 1", "[isl]")
+{
+	// 2 * intensity == cutoff * size^2 -> sqrt(0).
+	REQUIRE(ISLMath::CalculateRadius(0.025f, false, 1.0f, 1.0f) == Approx(1.0f));
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/cpp/test_isl_radiusmath.cpp` around lines 40 - 62, Add a regression
test for the exact-zero radius boundary by adding a TEST_CASE in
tests/cpp/test_isl_radiusmath.cpp that calls ISLMath::CalculateRadius with
parameters that would produce an exact zero radius and asserts the result is
clamped to 1.0f; reference ISLMath::CalculateRadius to locate the implementation
and mirror the style of the existing "CalculateRadius clamps a NaN" test so this
covers the zero-radius crash path once CalculateRadius is hardened. Ensure the
test is focused and uses REQUIRE(... == Approx(1.0f)).

src/Features/LightLimitFix/ShadowCasterMath.h (1)

24-35: 💤 Low value

P90 index calculation is off by one.

int(n * 0.9f) for n=10 yields index 9 (the maximum), but P90 should select the 9th smallest (index 8). This returns a value closer to the 100th percentile for typical sample sizes.

Consider int((n - 1) * 0.9f) or std::max(0, static_cast<int>(n * 0.9f) - 1) depending on the desired interpolation behavior.

♻️ Suggested fix

 	const int n = std::min(count, Window);
 	float tmp[Window];
 	std::copy(ring, ring + n, tmp);
-	const int idx = static_cast<int>(n * 0.9f);
+	const int idx = std::max(0, static_cast<int>(n * 0.9f) - 1);
 	std::nth_element(tmp, tmp + idx, tmp + n);
 	return tmp[idx];

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Features/LightLimitFix/ShadowCasterMath.h` around lines 24 - 35, The P90
index in FrameTimePercentile90 is off-by-one: replace the current idx =
static_cast<int>(n * 0.9f) with a computation that clamps to the valid [0, n-1]
range and uses (n-1) for percentile selection (for example idx = std::max(0,
static_cast<int>((n - 1) * 0.9f))) so nth_element(tmp, tmp + idx, tmp + n) picks
the correct 90th percentile sample; ensure idx is an int and stays within bounds
before the nth_element call in the FrameTimePercentile90 template.

src/Hooks.cpp (1)

1024-1030: Consider renaming the PR to match the shipped behavior.

These hook sites add particle-light render-pass culling, so the current refactor(slf): extract Tier-2 testable helpers title is misleading. Something like feat(lightlimitfix): cull particle light passes would be easier to trace. If this closes tracked work, add Closes #... or Addresses #....

As per coding guidelines: provide suggestions for conventional commit titles and issue references when the current title does not describe the code changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Hooks.cpp` around lines 1024 - 1030, The PR title should be updated to
reflect the actual change: these hooks (stl::write_thunk_call calls for
BSBatchRenderer_RenderPassImmediately1, BSBatchRenderer_RenderPassImmediately2,
BSBatchRenderer_RenderPassImmediately3 targeting
BSBatchRenderer::RenderPassImmediately) implement particle-light render-pass
culling, so rename the PR to a conventional commit like "feat(lightlimitfix):
cull particle light passes" (or "fix(light): cull particle-light render passes")
and, if this work closes an issue, add "Closes #<issue>" or "Addresses #<issue>"
to the description; update the PR title and body accordingly to match the
shipped behavior and include the issue reference.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/Features/LightLimitFix/SettingsSanitize.h (1)

15-18: ⚡ Quick win

Consider validating bounds or documenting preconditions.

The function correctly handles non-finite v by returning lo, but it doesn't validate that the bounds themselves are finite or that lo <= hi. If a caller passes NaN for lo, the function would return NaN (defeating its purpose). If hi < lo, std::clamp has undefined behavior (precondition violation in C++20).

Consider either:

1. Adding defensive checks: assert(std::isfinite(lo) && std::isfinite(hi) && lo <= hi)
2. Documenting the preconditions in a comment above the function

🛡️ Proposed defensive validation

 	// Clamp a user/config float to [lo, hi]. std::clamp passes NaN through
 	// unchanged (every NaN comparison is false), which would let a non-finite
 	// value reach the GPU and cause divisions / infinite loops / corruption, so
 	// reject non-finite inputs explicitly and fall back to the lower bound for
 	// degraded-but-stable behavior.
+	// Precondition: lo and hi must be finite, and lo <= hi.
 	inline float SanitizeFloat(float v, float lo, float hi)
 	{
+		assert(std::isfinite(lo) && std::isfinite(hi) && lo <= hi);
 		return std::isfinite(v) ? std::clamp(v, lo, hi) : lo;
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Features/LightLimitFix/SettingsSanitize.h` around lines 15 - 18,
SanitizeFloat currently checks only the input v but not the bounds; add
validation to ensure lo and hi are finite and lo <= hi (e.g., in SanitizeFloat
add an assertion like assert(std::isfinite(lo) && std::isfinite(hi) && lo <= hi)
or, alternatively, document the precondition above the function that callers
must provide finite bounds with lo <= hi); ensure the chosen approach prevents
std::clamp from receiving invalid bounds (or documents that responsibility) and
reference the SanitizeFloat function in the comment or assertion.

tests/cpp/test_shadowcaster_math.cpp (1)

41-47: ⚡ Quick win

Consider adding a count > Window case.

The existing tests cover count == 0, count == Window, and count < Window, but not count > Window — the branch where std::min(count, Window) clamps n and prevents an out-of-bounds copy. A regression that drops the clamp would read past ring; a test pins that contract.

💚 Suggested additional test

TEST_CASE("FrameTimePercentile90 clamps count above the window size", "[scm]")
{
	// count > Window: n is clamped to Window (10); all slots sampled.
	float ring[10] = { 5, 2, 9, 1, 7, 3, 10, 4, 8, 6 };
	REQUIRE(FrameTimePercentile90(ring, 99) == Approx(10.0f));
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/cpp/test_shadowcaster_math.cpp` around lines 41 - 47, Add a test that
validates FrameTimePercentile90 correctly clamps count above the Window size:
create a TEST_CASE that supplies a ring buffer of size Window (e.g., 10 values)
and calls FrameTimePercentile90(ring, 99) (or any count > Window) and REQUIREs
the expected percentile (e.g., Approx(10.0f) for the provided sample); this
ensures the std::min(count, Window) behavior is preserved and prevents
out-of-bounds reads in FrameTimePercentile90.